GroupBy[Series].count() return type should be Series[int] #966
Conversation
| @overload | ||
| def count(self: GroupBy[Series]) -> Series[int]: ... | ||
| @overload | ||
| def count(self: GroupBy[DataFrame]) -> DataFrame: ... |
There was a problem hiding this comment.
I'd prefer removing def count() from here, and having declarations in DataFrameGroupBy and SeriesGroupBy in core/groupby/generic.pyi
There was a problem hiding this comment.
I'm a new contributor to this repo and eager to learn more about these choices. Is there a specific reason for moving the declarations to core/groupby/generic.pyi?
In the actual pandas repo, the count() method is implemented in core/groupby/groupby.py, not core/groupby/generic.py.
Thanks in advance for taking the time to explain.
There was a problem hiding this comment.
I'm a new contributor to this repo and eager to learn more about these choices. Is there a specific reason for moving the declarations to
core/groupby/generic.pyi?In the actual pandas repo, the
count()method is implemented incore/groupby/groupby.py, notcore/groupby/generic.py.Thanks in advance for taking the time to explain.
It probably is due to that we had issues in the past with doing overloads where self was declared as Series vs. DataFrame, so putting the stubs in the lower classes was a way to get around that problem.
The type checkers have improved, so it's possible your approach could be used going forward.
Having said that, I see that for any() and all() in the file you changed, that was the pattern used, so maybe your change was fine the way it was!
As an example, there are a number of methods shared between Series and DataFrame that are implemented in NDFrame, but instead of putting the stub in NDFrame, we put them in Series and DataFrame to clarify that the results are different in the two subclasses.
|
We just uncovered an issue with our CI that was fixed earlier today. Can you merge with current |
|
I have merged |
|
Ugh, so now CI is failing because when you first tested, it used pyright 1.1.373 and that got locked for further tests. Can you include this in your PR? In to |
|
I have updated the pyright dependency version in |
assert_type()to assert the type of any return value